Skip to content

Binary match date strings#39

Merged
aseigo merged 9 commits into
masterfrom
refactor/binary-match-date-strings
Apr 13, 2026
Merged

Binary match date strings#39
aseigo merged 9 commits into
masterfrom
refactor/binary-match-date-strings

Conversation

@aseigo

@aseigo aseigo commented Apr 12, 2026

Copy link
Copy Markdown
Contributor

Issue: #32

This replaces uses of Timex.parse/2 in ICal.Deserialze with binary matching.

One nice nice side effect: the code longer creates a new string with the timezone just to parse it out again.

@aseigo aseigo self-assigned this Apr 12, 2026
@aseigo aseigo mentioned this pull request Apr 12, 2026
18 tasks

@matthewlehner matthewlehner left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! I just wanted to highlight some of the stuff I found here when looking at this yesterday.

Hopefully this is helpful!

Comment thread lib/ical/deserialize.ex Outdated

@matthewlehner matthewlehner left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good to me! (I should say, after adding support for gap calculations)

Love the change to tz and mint.

Comment thread lib/ical/deserialize.ex
# get UTC, then express in the post-gap offset (EDT) → 3:30 AM EDT.
case DateTime.new(date, time, timezone) do
{:ok, dt} -> dt
{:ambiguous, first, _second} -> first

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right, this is correct per the spec!

If, based on the definition of the referenced time zone, the local
time described occurs more than once (when changing from daylight
to standard time), the DATE-TIME value refers to the first
occurrence of the referenced time.

Comment thread lib/ical/deserialize.ex Outdated
case DateTime.new(date, time, timezone) do
{:ok, dt} -> dt
{:ambiguous, first, _second} -> first
{:gap, _just_before, just_after} -> just_after

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, if only this could be simple!

The gap requires a calculating when the occurrence would be based on UTC, then putting it into the timezone after.

Something like this:

Suggested change
{:gap, _just_before, just_after} -> just_after
{:gap, just_before, _just_after} ->
pre_gap_offset = just_before.utc_offset + just_before.std_offset
naive = NaiveDateTime.new!(date, time)
utc = NaiveDateTime.add(naive, -pre_gap_offset, :second)
DateTime.shift_zone!(DateTime.from_naive!(utc, "Etc/UTC"), timezone)

The spec says:

2:30 AM in a spring-forward gap → apply pre-gap offset (EST) to
get UTC, then express in the post-gap offset (EDT) → 3:30 AM EDT.

Super glad we're handling this 😂

it might be worth extracting this to a private function or adding a comment here just to make it clear why this is the way it is.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The gap requires a calculating when the occurrence would be based on UTC

It's even easier: the "gapped" time is somewhere in the gap. So one needs to calculate the offset from where the discontinuity starts (e.g. spring forward at 02:00) to the gapped time, then shift the actual time (after the gap ends) by that amount.

So 02:30 in a gap of 02:00 -> 03:00 becomes 3:00 + (2:30 - 2:00). Resolved in 7fd6f35

gap times return the before/after moments of the gap. our time is
somewhere in that gap. so shift the after by the amount the gapped
time would have been after the start
@aseigo aseigo force-pushed the refactor/binary-match-date-strings branch from 2987cc9 to 7fd6f35 Compare April 13, 2026 07:38
@aseigo aseigo requested a review from matthewlehner April 13, 2026 07:41
@aseigo aseigo merged commit 3a7c2cb into master Apr 13, 2026
5 checks passed
@aseigo aseigo deleted the refactor/binary-match-date-strings branch April 13, 2026 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants